fix(graphile-postgis): wrap GeoJSON spatial filter inputs with ST_GeomFromGeoJSON (#724)#990
Merged
pyramation merged 2 commits intomainfrom Apr 17, 2026
Merged
Conversation
… spatial operators Fixes #724. All 26 PostGIS spatial operators registered by createPostgisOperatorFactory() were relying on the default resolveSqlValue pipeline, which binds the GeoJSON object as raw text cast to ::geometry / ::geography. Postgres' geometry_in and geography_in parsers do not accept GeoJSON text — they require ST_GeomFromGeoJSON() wrapping. Any spatial filter with a GeoJSON input therefore failed at runtime with 'parse error - invalid geometry'. Every spatial operator now overrides resolveSqlValue to sql.null and wraps the input with ST_GeomFromGeoJSON($1::text) in the resolve function, with an optional ::geography cast for geography-based operators. This matches the pattern already used correctly by within-distance-operator.ts. Unit tests in connection-filter-operators.test.ts are updated to assert the new ST_GeomFromGeoJSON wrapping contract. The broader regression suite (graphql/orm-test/__tests__/postgis-spatial.test.ts) lands via PR #989 and flips green once this change is merged.
Contributor
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #724. All 26 PostGIS spatial operators registered by
createPostgisOperatorFactory()were relying on the default connection-filterresolveSqlValuepipeline, which binds the GeoJSON object as raw text cast to::geometry/::geography. PostgreSQL'sgeometry_in/geography_inparsers do not accept GeoJSON text — they requireST_GeomFromGeoJSON(...)wrapping. Every spatial filter with a GeoJSON input therefore failed at runtime withparse error - invalid geometry.The change, per operator registration:
resolveSqlValue: () => sql.nullto disable the default bind.resolve(...), wrap the input via<schema>.st_geomfromgeojson($1::text), appending::<schema>.geographywhen the registration is on a geography-based type.This is the same pattern already used correctly by
within-distance-operator.ts— now applied consistently across the broader operator set.Unit tests in
connection-filter-operators.test.tsare rewritten to assert the new contract: every operator's compiled SQL must contain the schema-qualifiedst_geomfromgeojson($1::text)wrap, and geography-type registrations must additionally append the::geographycast.The full end-to-end regression suite (live PG, 66 tests across all spatial operators × 7 GeoJSON input shapes × both codecs) lands via PR #989 and flips green once this is merged.
Updates since last revision
graphile/graphile-postgisto the CI matrix in.github/workflows/run-tests.yamlso the ST_GeomFromGeoJSON wrapping contract (the 218-test unit suite for this package, now including the new assertions) runs on every PR — previously the package was not in the matrix, which is how fix: bypass grafast middleware in graphile-test to preserve adapted withPgClient #724 slipped through in the first place. All 47 matrix jobs pass on this PR head.Review & Testing Checklist for Human
spec.baseType === 'geography'appends::<schema>.geographyafter theST_GeomFromGeoJSON(...)wrap. Verify this produces the geography overload of eachST_*function rather than the geometry one, especially for operators registered on both codecs (intersects, covers, coveredBy, exactlyEquals, bboxIntersects2D).sql.identifier(schemaName, 'st_geomfromgeojson')andsql.identifier(schemaName, 'geography')resolve correctly when PostGIS is installed to a non-publicschema — there's a unit test for apostgisschema, but no live integration test in this PR. (PR test(postgis): regression coverage for GeoJSON spatial filter binding (#724) #989 exercises live PG but assumes the defaultpublicschema layout.)geometry-typed bind via its own pipeline) to confirm they haven't regressed. The default pipeline is now disabled for all 26 operators.Notes
WithinDistanceInputis registered by graphile-postgis but does not surface on the generatedGeometryInterfaceFilterschema type. This is unrelated to the GeoJSON binding bug and is tracked as a follow-up; the two test cases in the regression suite that exercisewithinDistanceare markedxitwith a FIXME trail.mainuntil this fix is merged and then rebased → green.Link to Devin session: https://app.devin.ai/sessions/c5eeee65a3c546c4ac6753bb05fa03e0
Requested by: @pyramation